Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround pybind11 bug on Windows when CMAKE_BUILD_TYPE=RelWithDebInfo #538

Merged
merged 11 commits into from
Oct 14, 2020

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Oct 9, 2020

Another workaround for a pybind11 bug ...

I should start sending patches upstream (for this and #504).
When that gets included in a new pybind11 release, we can update our vendor package and get rid of these workarounds (I will open an issue to track that).

Though it failed in a packaging build, only -DCMAKE_BUILD_TYPE=RelWithDebInfo is needed to reproduce the issue:

  • Windows RelWithDebInfo Build Status

TODO: This still doesn't pass, I have to check more closely in a Windows VM. CI is passing now.

Fixes #537.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the bug Something isn't working label Oct 9, 2020
@ivanpauno ivanpauno self-assigned this Oct 9, 2020
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

This should pass now:

  • Windows RelWithDebInfo Build Status

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

ivanpauno commented Oct 9, 2020

This patch is terrible bad but:

  • Debug and RelWithDebInfo build types are using /INCREMENTAL link flag on Windows (see here).
  • pybind11 is forcing /LTCG and /GL, but when build type is RelWithDebInfo it shouldn't be doing that (see comment and links in the diff).

Without a proper fix in pybind11 I don't see another way to fix this.
Ignoring the warning is also possible, but in that case MSVC will be using "non standard" optimization options for a RelWithDebInfo build.

@ivanpauno ivanpauno marked this pull request as ready for review October 9, 2020 20:26
@ivanpauno ivanpauno requested a review from a team as a code owner October 9, 2020 20:26
@ivanpauno ivanpauno requested review from thomas-moulard, jikawa-az and nuclearsandwich and removed request for a team October 9, 2020 20:26
@nuclearsandwich nuclearsandwich removed their request for review October 9, 2020 21:16
@nuclearsandwich
Copy link
Member

I'm not really in a position to evaluate the fitness here but I appreciate the head's up. Hopefully it's something we can push upstream in short order.

@ivanpauno ivanpauno requested a review from a team October 9, 2020 21:20
@ivanpauno
Copy link
Member Author

I removed the ros2/tooling-reviewersreview request by accident, I've added it back.

@jacobperron
Copy link
Member

It would be good to fix this upstream, @ivanpauno are you able open a PR and reference it in this change so that we can revert it after an upstream release?

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this LGTM.

Looks like this will be fixed in the next pybind11 release (2.6.0). I'll keep my eyes open for it and update our vendor package when it's out.

@ivanpauno
Copy link
Member Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit f50498a into master Oct 14, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/rosbag2_py_rel_with_deb_info branch October 14, 2020 19:07
jacobperron added a commit that referenced this pull request Oct 22, 2020
emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
…nfo (#538)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
sloretz added a commit to ros2/rclpy that referenced this pull request Feb 16, 2021
Copies windows debug fixes from rosbag2_py for using Pybind11 on Windows
debug and RelWithDebInfo.

For more info see the original PRs

ros2/rosbag2#538
ros2/rosbag2#531

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
…nfo (#538)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
sloretz added a commit to ros2/rclpy that referenced this pull request Feb 23, 2021
Copies windows debug fixes from rosbag2_py for using Pybind11 on Windows
debug and RelWithDebInfo.

For more info see the original PRs

ros2/rosbag2#538
ros2/rosbag2#531

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
sloretz added a commit to ros2/rclpy that referenced this pull request Feb 24, 2021
* Copy windows debug fixes for pybind11

Copies windows debug fixes from rosbag2_py for using Pybind11 on Windows
debug and RelWithDebInfo.

For more info see the original PRs

ros2/rosbag2#538
ros2/rosbag2#531

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Workaround for consistent Py_DEBUG macro

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* fix argument order

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Does PUBLIC Py_DEBUG resolve differences?

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Don't include Python.h if including pybind11.h

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Remove Py_DEBUG if target doesn't use pybind11

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New warnings in Windows packaging jobs
3 participants